Bigquery #455

drewbanin · 2017-06-07T15:08:29Z

-- profile.yml
bq:
  target: dev
  outputs:
    dev:
      type: bigquery
      method: oauth
      project: 'project_name'
      schema: "your_schema" # dataset
      threads: 1

cmcarthur

in addition to comments here, this needs:

unit tests on completely new functionality (adapter task)
integration tests on BQ

cmcarthur · 2017-06-12T13:22:49Z

dbt/adapters/bigquery.py

+    @classmethod
+    def get_bigquery_credentials(cls, config):
+        method = config.get('method')
+        Creds = google.oauth2.service_account.Credentials


don't upcase variable names

cmcarthur · 2017-06-12T13:23:01Z

dbt/adapters/bigquery.py

+        Creds = google.oauth2.service_account.Credentials
+
+        if method == 'oauth':
+            return None


what happens if they use the oauth method?

We initialize a bigquery client with the credentials argument if it's a service account (from a private key file or the private key raw json).

If the credentials object is None (as with oauth), then the bq client lib will use the oauth flow to authenticate the user. We force this flow if the user isn't authenticated by shelling out to gcloud here

More info on gcp auth flows here

cmcarthur · 2017-06-12T13:23:15Z

dbt/adapters/bigquery.py

+            return Creds.from_service_account_info(details)
+
+        error = ('Bad `method` in profile: "{}". '
+                 'Should be "oauth" or "service-account"'.format(method))


or service-account-json?

also can we put this validation into a voluptuous schema for the bigquery profile?

cmcarthur · 2017-06-12T13:24:36Z

dbt/adapters/bigquery.py

+                  from_name, to_name)
+        raise dbt.exceptions.NotImplementedException(message)
+
+    # hack because of current API limitations


can you elaborate more? if there's a link explaining why we need this, you should add it here

Sure, it's actually not super well documented which is part of the problem. BQ defaults to the "Legacy SQL" syntax when views are created via the API. You can set a useLegacySql = False flag on the Table object, but it looks like it's presently ignored by the API.

Relevant issue

The best workaround i could find was to use the #standardSQL header line

cmcarthur · 2017-06-12T13:27:04Z

dbt/adapters/bigquery.py

+        _, cursor = cls.add_query(profile, sql, 'master')
+        res = cursor.fetchone()
+
+        logger.debug("Cancel query '{}': {}".format(connection_name, res))


what's the behavior on ctrl+c here?

if we need to just raise an exception, you should remove all this code.

also please change this and the exception for add_query to dbt.exceptions.NotImplementedException

@cmcarthur hadn't gotten there yet :)

Looks like we can actually issue a cancel command for running queries: https://cloud.google.com/bigquery/docs/reference/rest/v2/jobs/cancel

@cmcarthur it doesn't look like it's possible to cancel a table/view creation request. Jobs can be canceled though, which may be good to keep in mind for the future. This branch will log an error that looks like this on ctrl-c:

The bigquery adapter does not support query cancellation. Some queries may still be running!

cmcarthur · 2017-06-12T13:28:15Z

dbt/adapters/default.py

+        for package, require in cls.requires.items():
+            logger.info("Installing {}".format(require))
+            pip_main(['install', require])
+            logger.info("Installed {} successfully!".format(require))


is there a unit test for this? i want to be sure it works on all platforms

cmcarthur · 2017-06-12T13:30:18Z

dbt/contracts/connection.py

@@ -33,10 +33,19 @@
    Optional('role'): basestring,
 })

+bigquery_credentials_contract = Schema({
+    Required('method'): basestring,


All(basestring, Any(['oauth', 'service-account', 'service-account-json']))

Can i just do one of these:

Any(['oauth', 'service-account', 'service-account-json'])

cmcarthur · 2017-06-12T13:31:11Z

dbt/logger.py

@@ -44,6 +43,7 @@


 def make_log_dir_if_missing(log_dir):
+    import dbt.clients.system
    dbt.clients.system.make_directory(log_dir)


why move this import in here?

the logger imports the system client, and the system client imports the logger. This breaks the circular import

cmcarthur · 2017-06-12T13:35:26Z

test/unit/test_runner.py


        dbt.adapters.postgres.PostgresAdapter.drop.assert_not_called()

        mock_adapter_truncate.assert_not_called()
-        mock_adapter_rename.assert_not_called()
+        mock_adapter_rename.assert_called_once()


this makes me nervous. was this not working properly before? my understanding that view models would just be created with the final name if they did not previously exist (vs created and then renamed)

i'm really dissatisfied with our current drop/create/rename logic. It works ok, but that __dbt_tmp suffix makes a lot of coordination between dbt components unduly difficult. I think this was an attempt at simplifying how it works, but let me revisit. Good catch

drewbanin · 2017-06-16T16:57:59Z

@cmcarthur this is pretty much good. I did change the way views are materialized (temp table, then renamed), so the changed unit test is warranted imo.

I have a single integration test for BQ which passes on circle and appveyor.

Think this is ready for another look!

cmcarthur

@drewbanin this looks ok to ship. i did add one more comment.

cmcarthur · 2017-06-19T17:11:27Z

dbt/adapters/bigquery.py

+
+    @classmethod
+    def get_status(cls, cursor):
+        raise Exception("Not implemented")


NotImplementedException // remove code below

good catch 👍

* wip * minimum viable bq adapter * proper bq quoting * absolute madness * auto-install bq module * catch runtime exception for nodes * cleanup * more cleanup * pip critical logging * add adapter command * major wip * refactorin * closer * handle model errors + dependent skips * cleanup + test transactions (for now) * move model creation to materializers * fix for ephemeral models * override materializer for bq * error handling * bq tests working * commit tweaks for models * service accounts * service account json for bq * better error message if adapter is not installed * fix unit tests * pep8 * fix integration tests * codeclimate * fix typos * fix circular dep for imports * catch programming exception for runners * code review changes * refactoring for code climate * selector cleanup * fix bug for erin * comment * handle cancellation on ctrl-c for bq (log warning) * better bq validation * test bq validation * add uninstall flag to adapter task * remove pip hacking nonsense * bq integration tests * remove initialize call for pg * fix bq integration tests * pep8 * remove -x opt from toxfile * handle notimplemented for bq better * missing import for seed task * notimplemented for bq automatic commit by git-black, original commits: 8b8b3aa

drewbanin added 30 commits May 29, 2017 17:50

wip

b2781b9

Merge branch 'development' into bigquery

ba28422

minimum viable bq adapter

f5b70ac

Merge branch 'development' into bigquery

3e97021

proper bq quoting

dfdb6e4

absolute madness

c4802db

Merge branch 'adapter-agnostic' into bigquery

d82eba2

auto-install bq module

17b330b

catch runtime exception for nodes

5d3ca1e

cleanup

f3ce484

more cleanup

ba53a7a

pip critical logging

db99614

add adapter command

d449717

Merge branch 'adapter-agnostic' into bigquery

e5008bd

major wip

b52af47

refactorin

150e4eb

closer

6b92e1b

handle model errors + dependent skips

1e0e989

cleanup + test transactions (for now)

79a7759

move model creation to materializers

ddb1296

fix for ephemeral models

28f61b8

override materializer for bq

173a1b7

error handling

ac356c3

bq tests working

3bd2e61

commit tweaks for models

1c4ed3c

service accounts

baa542f

service account json for bq

6f3a1a2

better error message if adapter is not installed

b2042ae

fix unit tests

a9cf033

pep8

a066798

drewbanin added 4 commits June 9, 2017 15:23

codeclimate

75e2ba6

fix typos

a644e5d

fix circular dep for imports

bf10de9

catch programming exception for runners

65a872a

cmcarthur requested changes Jun 12, 2017

View reviewed changes

drewbanin added 14 commits June 12, 2017 10:16

code review changes

29c1f8b

refactoring for code climate

48321d6

selector cleanup

8049b17

fix bug for erin

b5d44e6

comment

fc02cfa

handle cancellation on ctrl-c for bq (log warning)

4f8d2bd

better bq validation

9849fa0

test bq validation

349c12e

add uninstall flag to adapter task

397dc9c

remove pip hacking nonsense

199cf0e

bq integration tests

612eef9

remove initialize call for pg

f5aa68a

fix bq integration tests

c222fc2

pep8

c2fc1e1

drewbanin mentioned this pull request Jun 16, 2017

implement support for bigquery #438

Closed

8 tasks

drewbanin added the code review label Jun 16, 2017

drewbanin added 3 commits June 16, 2017 13:51

remove -x opt from toxfile

3c943a9

handle notimplemented for bq better

11c831a

missing import for seed task

a2b7af6

cmcarthur approved these changes Jun 19, 2017

View reviewed changes

notimplemented for bq

b3065ef

drewbanin merged commit 8b8b3aa into development Jun 19, 2017

drewbanin deleted the bigquery branch June 19, 2017 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bigquery #455

Bigquery #455

drewbanin commented Jun 7, 2017 •

edited

Loading

cmcarthur left a comment

cmcarthur Jun 12, 2017

cmcarthur Jun 12, 2017

drewbanin Jun 12, 2017

cmcarthur Jun 12, 2017

cmcarthur Jun 12, 2017

cmcarthur Jun 12, 2017

drewbanin Jun 12, 2017 •

edited

Loading

cmcarthur Jun 12, 2017

drewbanin Jun 12, 2017

drewbanin Jun 15, 2017 •

edited

Loading

cmcarthur Jun 12, 2017

cmcarthur Jun 12, 2017

drewbanin Jun 12, 2017

cmcarthur Jun 12, 2017

drewbanin Jun 12, 2017

cmcarthur Jun 12, 2017

drewbanin Jun 12, 2017

drewbanin commented Jun 16, 2017

cmcarthur left a comment

cmcarthur Jun 19, 2017

drewbanin Jun 19, 2017

Bigquery #455

Bigquery #455

Conversation

drewbanin commented Jun 7, 2017 • edited Loading

cmcarthur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewbanin Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewbanin Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewbanin commented Jun 16, 2017

cmcarthur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewbanin commented Jun 7, 2017 •

edited

Loading

drewbanin Jun 12, 2017 •

edited

Loading

drewbanin Jun 15, 2017 •

edited

Loading